Conversation
watchman.rb
Outdated
There was a problem hiding this comment.
If you have an array where you want to do something to each element, I'd rather you limit your collection first, rather than loop over everything and selectively do something. It will lead to blocks that you can reuse and extract to methods, and it reads your intent easier.
network.shows.each do |show|
puts show if show.day_of_week.downcase == day_of_week
endvs
network.shows.select{ |show| show.day_of_week.downcase == day_of_week }.each do |show|
puts show
endThere was a problem hiding this comment.
...limit your collection first, rather than loop over everything and selectively do something. It will lead to blocks that you can reuse and extract to methods, and it reads your intent easier.
That makes a ton of sense. Thanks!
Would you recommend assigning network.shows.select{ |show| show.day_of_week.downcase == day_of_week } to a variable for readability? That line is 87 chars.
There was a problem hiding this comment.
Yes, that would work. Or you can have a method:
def for_day_of_week(shows, day)
shows.select{ |show| show.day.downcase == day }
end
for_day_of_week(network.shows, day_of_week).each do |show|
puts show
endAnd, we could get even fancier with the last. The naming "for_day_of_week" could be "each_day_of_week", implying we give it a block (though it is fairly advanced, and you'd only do this if you needed to re-use each_day_of_week)
def each_day_of_week(shows, day)
shows.select{ |show| show.day.downcase == day }.each do |show|
yield show
end
end
each_day_of_week(network.shows, day_of_week) do |show|
puts show
end|
@jwo added my Eagle level submission. Thanks! |
watchman.rb
Outdated
There was a problem hiding this comment.
Dont think you need to keep this, right?
No description provided.